fix: handle undefined values in answer lists #2204

jfmcquade · 2024-02-14T15:07:34Z

PR Checklist

PR title descriptive (can be used in release notes)

Description

Handles the case where some values for text or image within an answer list may be undefined. When authoring an answer list that gets dynamically populated by various data lists, there will sometimes be undefined values that are complicated to handle at an authoring level (#2166). For example, for a generated answer list of length 4:

image/text values for some items may be blank
there may be fewer than 4 options (i.e. for some items both text and image is blank)

This PR adds answer list parsing such that:

in case 1., the image/text component corresponding to an undefined image/text property does not render (previously a broken image link or the word "undefined" would be shown)
In case 2., where an item in the answer list has no value for text or image, the whole item is not rendered.

This fix applies to all components that take answer_list parameters: combo_box, radio_button_grid and radio_group. See demos below.

Testing

Test the debug_answer_list_partial template (see screenshots below). Also consider testing example_answer_data, which was added in #1967 (the summer of love) to test answer lists coming directly from data lists, to ensure this still functions as expected.

Git Issues

Closes #2166

Screenshots/Videos

Template debug_answer_list_partial:

Data list debug_answer_list_partial_data:

debug_answer_list_partial demo.

Note that in this demo, the combo_box and radio_grid components displays a blank box for option_3. This is because option_3 does have a value for image in the answer list, and only those items with no value for all other fields besides name will be removed from the answer list. In actual applications, answer lists passed to a combo_box and radio_grid components would not typically contain any properties besides name and text (there would be no image). If we do want to handle undefined values in the case of an answer list that will be used for both combo_box and, for example, radio_button_grid components, that can be handled in a follow-up.

answer.list.partial.demo.mov

chrismclarke

I have a couple reservations when it comes to implementing in this way, see comments inline - happy to discuss on a call if useful

chrismclarke · 2024-02-14T23:52:15Z

src/app/shared/utils/utils.ts

      if (field && value) {
+        if (["undefined", "NaN", "null", '""'].includes(value)) value = undefined;


nit(blocking)
Do you have any idea how a string "undefined" "NaN" or "null" could be populated? It would be much better to fix at the source, e.g. if there is some sort of getStringParam parsing these then we should apply the fix there instead. I'm guessing similarly we need a sensible response for NaN which I'm guessing comes from some parseNumberParam` or similar...

Additionally I'm not sure converting empty string to undefined makes sense. When thinking of answer lists there's sometimes a case where html components may want to use empty string to represent nothing selected in the context of something like a select dropdown.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select

Yeh I don't think this was sensible, I've updated to only convert the string "undefined" to undefined. It is supposed to catch precisely the case where the value is undefined because the answer list is being generated from a data list which may not have a value in the relevant column

chrismclarke · 2024-02-15T00:16:18Z

src/app/shared/utils/utils.ts

+  // e.g. "image" and "text" are undefined because the list has been generated within a template
+  const filteredAnswerListItems = answerListItems.filter((item: IAnswerListItem) => {
+    for (let [key, value] of Object.entries(item)) {
+      if (key !== "name" && value !== undefined) return true;


nit(blocking)
This logic seems a bit confusing to me, we're running a filter operation but then have an inner for loop which will escape before completion. Might make more sense as 2-step operation, e.g more like

const hadItemData = Object.entries(item).find(([key,value])=>(key !== "name" && value !== undefined) return hadItemData ? true : false

That being considered, I'm not sure if this makes total sense to implement - surely it's better to let broken images through so that an author can be aware of an authoring mistake instead of just hiding the data away? I'd probably lean more towards providing an authoring option to add a filter function to the specific component, e.g. filter_no_image:true or ensuring support for runtime filtering, e.g. @data.some_list.filter(item=>item.image!==undefined)

Additionaly we'd still end up with broken images in cases where text is supplied without image with the above filter.

Your suggested logic is definitely clearer, I have refactored accordingly.

In terms of allowing through broken images, a broken image link will still be allowed through with the suggested implementation – an invalid path to an image will still display a broken image link. The image will only be filtered out if the value is undefined – which admittedly could be due to an authoring error that the author should be made aware of, but it's not clear to me that displaying a broken image would be necessary in such cases.

Either way, returning to the issue referenced in this PR, we want a way to author an answer list in a generic way where the source data may or may not contain image paths, and in the case where an item does not reference an image, to display no image (and not a broken image link). Filtering on the undefined values as suggested seems like a satisfactory solution, but I may be missing some associated problems, and there may be other approaches.

Ultimately, I think the best solution may be a way of defining arbitrary data structures, or at least answer lists, directly from data lists, handled at parse-time, in a way that can handle lists of multiple length and containing partial values (as in WIP RFC for a universal data list parser). But I think we want an acceptable solution sooner in order to help with the current authoring requirements.

chrismclarke · 2024-02-21T04:51:49Z

Just to quickly summarise from the call today

Seems like a reasonable option to remove entries that have no data included whatsover
Existing logic should be able to handle image/text removal if one if included and not the other (to be confirmed?)
It would still be preferable to track down how "undefined" string ends up in the answer_list if possible (to reduce chance of similar issue re-occuring), or to create as a small follow-up issue if not easy to do (and apply workaround proposed in pr)

…empla teRow util function

jfmcquade · 2024-02-21T16:01:35Z

It would still be preferable to track down how "undefined" string ends up in the answer_list if possible (to reduce chance of similar issue re-occuring), or to create as a small follow-up issue if not easy to do (and apply workaround proposed in pr)

I think this is actually quite straightforward: "undefined" would be a string in answer list in cases like that in the issue referenced in this PR (e.g. when explicitly defining an answer list referring to values which themselves may be undefined) because the answer list string does not receive any parsing additional to the parseAnswerList function.

#1967 introduced the parseAnswerList utility function, where previously components that used answer lists handled the parsing directly in the component code.

I've now added a commit to this PR, which incorporates this logic into a getAnswerListParamFromTemplateRow() utility function to closer resemble the other utility functions for getting params.
As an answer list before this parsing is just an array of strings, it is not surprising that the value "undefined" would come through as a string. The fix is exactly what is now included in this PR: explicitly converting "undefined" to undefined when it is the value of a property within an entry of an answer list. It may be that we want to do additional conversions on some more strings here (e.g. "null"), but it's not apparent to me how these would end up in the answer list string, and all of the properties of an answer list item are expected to be strings (so we don't need to parse numbers as such), so I think handling "undefined" should be fine for now.

chrismclarke · 2024-02-21T17:07:06Z

I think this is actually quite straightforward: "undefined" would be a string in answer list in cases like that in the issue referenced in this PR (e.g. when explicitly defining an answer list referring to values which themselves may be undefined) because the answer list string does not receive any parsing additional to the parseAnswerList function.

Ah - yeah I see now, yeah the answerlist is usually stored in an intermediate local variable which is an array (auto converted by parser for anything with _list in name), but whose elements are formatted string. So when the dynamic variables are replaced they are dropped into something like
["name: @local.name | text: @local.text"] -> ["name: undefined | text:undefined"]

So what would actually be better would be to process these within the parser to convert the string-array into an object-array. That way when replacing the localVariable it could already be formatted as:

[{
   "name": "@local.name",
   "text": "@local.text"
}]

Which when converted should replace the variables (local, data, field or anything else) it should be able to assign the proper value.

I think would make sense to merge what's here now to fix the issue in the short term, and then I'll try make a quick follow-up pr/issue to see if possible to update the parser as needed.

chrismclarke · 2024-02-21T17:09:15Z

src/app/shared/utils/utils.ts

+  _default: IAnswerListItem[]
+): IAnswerListItem[] {
+  const params = row.parameter_list || {};
+  console.log(params[name]);


nit(non-blocking) can remove temp logs

chrismclarke

Thanks @jfmcquade

I think looks good to fix the immediate issue (have suggested follow-up in separate comment). I think also makes sense to have the more unified way to extract answer list param so thanks for adding

chrismclarke · 2024-02-21T17:11:52Z

src/app/shared/utils/utils.ts

+ * (possibly still in string representation) or a data list (hashmap of IAnswerListItems)
+ */
+function parseAnswerList(answerList: any) {
+  if (!answerList) return [];


nice addition (although shame the git diff isn't showing very cleanly)
I think the rest of the code is the same as you updated previously, so looks good to me

esmeetewinkel

Functional test passed (debug and plh_kids_tz).

I'm seeing the parsed answer lists as red text (not errors though) in the console which I think can be helpful for debugging.

jfmcquade · 2024-02-22T11:01:11Z

I think would make sense to merge what's here now to fix the issue in the short term, and then I'll try make a quick follow-up pr/issue to see if possible to update the parser as needed.

I've created an issue here: #2210

I'm seeing the parsed answer lists as red text (not errors though) in the console which I think can be helpful for debugging.

This was actually a log I didn't mean to leave in. If it, or a similar log, would be useful to keep then we should probably add a label like Source answer_list:

esmeetewinkel · 2024-02-22T11:28:28Z

This was actually a log I didn't mean to leave in. If it, or a similar log, would be useful to keep then we should probably add a label like Source answer_list:

Okay, I guess it would be more consistent with the rest of the app to leave it out - and the console would be less cluttered. This does remind me of our discussion that it would be useful for debugging to "see" the values of local variables somehow.

Would it be possible/sensible to have logs of this sort visible in the console when the debug-mode is enabled?

fix: handle undefined values in answer lists

e918207

github-actions bot added the fix label Feb 14, 2024

jfmcquade temporarily deployed to debug February 14, 2024 15:07 — with GitHub Actions Inactive

github-actions bot added fix and removed fix labels Feb 14, 2024

jfmcquade requested review from chrismclarke and esmeetewinkel February 14, 2024 15:13

chrismclarke requested changes Feb 15, 2024

View reviewed changes

fix: only convert undefined to undefined in answer list parsing

c7732ba

github-actions bot added fix and removed fix labels Feb 20, 2024

jfmcquade temporarily deployed to debug February 20, 2024 11:41 — with GitHub Actions Inactive

refactor: filteredAnswerListItems logic

082196b

github-actions bot added fix and removed fix labels Feb 20, 2024

jfmcquade temporarily deployed to debug February 20, 2024 11:47 — with GitHub Actions Inactive

esmeetewinkel removed their request for review February 20, 2024 16:40

Merge branch 'master' into fix/combo-box-radio-group-empty-fields

942fbbb

github-actions bot added fix and removed fix labels Feb 21, 2024

jfmcquade temporarily deployed to debug February 21, 2024 15:08 — with GitHub Actions Inactive

refactor: incorporate parseAnswerList into new getAnserListParamFromT…

265c30c

…empla teRow util function

github-actions bot added fix and removed fix labels Feb 21, 2024

jfmcquade temporarily deployed to debug February 21, 2024 15:54 — with GitHub Actions Inactive

jfmcquade requested review from chrismclarke and esmeetewinkel February 21, 2024 16:02

github-actions bot added fix and removed fix labels Feb 21, 2024

chrismclarke reviewed Feb 21, 2024

View reviewed changes

chrismclarke approved these changes Feb 21, 2024

View reviewed changes

esmeetewinkel approved these changes Feb 21, 2024

View reviewed changes

esmeetewinkel merged commit 1ffd258 into master Feb 21, 2024
7 checks passed

esmeetewinkel deleted the fix/combo-box-radio-group-empty-fields branch February 21, 2024 18:20

jfmcquade mentioned this pull request Feb 22, 2024

[FEATURE] Process answer lists within the parser #2210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle undefined values in answer lists #2204

fix: handle undefined values in answer lists #2204

jfmcquade commented Feb 14, 2024 •

edited

Loading

chrismclarke left a comment

chrismclarke Feb 14, 2024

jfmcquade Feb 20, 2024

chrismclarke Feb 15, 2024 •

edited

Loading

jfmcquade Feb 20, 2024

chrismclarke commented Feb 21, 2024 •

edited

Loading

jfmcquade commented Feb 21, 2024 •

edited

Loading

chrismclarke commented Feb 21, 2024 •

edited

Loading

chrismclarke Feb 21, 2024

chrismclarke left a comment

chrismclarke Feb 21, 2024

esmeetewinkel left a comment •

edited

Loading

jfmcquade commented Feb 22, 2024

esmeetewinkel commented Feb 22, 2024 •

edited

Loading

		if (field && value) {
		if (["undefined", "NaN", "null", '""'].includes(value)) value = undefined;

fix: handle undefined values in answer lists #2204

fix: handle undefined values in answer lists #2204

Conversation

jfmcquade commented Feb 14, 2024 • edited Loading

Description

Testing

Git Issues

Screenshots/Videos

Template debug_answer_list_partial:

Data list debug_answer_list_partial_data:

debug_answer_list_partial demo.

chrismclarke left a comment

Choose a reason for hiding this comment

chrismclarke Feb 14, 2024

Choose a reason for hiding this comment

jfmcquade Feb 20, 2024

Choose a reason for hiding this comment

chrismclarke Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

jfmcquade Feb 20, 2024

Choose a reason for hiding this comment

chrismclarke commented Feb 21, 2024 • edited Loading

jfmcquade commented Feb 21, 2024 • edited Loading

chrismclarke commented Feb 21, 2024 • edited Loading

chrismclarke Feb 21, 2024

Choose a reason for hiding this comment

chrismclarke left a comment

Choose a reason for hiding this comment

chrismclarke Feb 21, 2024

Choose a reason for hiding this comment

esmeetewinkel left a comment • edited Loading

Choose a reason for hiding this comment

jfmcquade commented Feb 22, 2024

esmeetewinkel commented Feb 22, 2024 • edited Loading

jfmcquade commented Feb 14, 2024 •

edited

Loading

chrismclarke Feb 15, 2024 •

edited

Loading

chrismclarke commented Feb 21, 2024 •

edited

Loading

jfmcquade commented Feb 21, 2024 •

edited

Loading

chrismclarke commented Feb 21, 2024 •

edited

Loading

esmeetewinkel left a comment •

edited

Loading

esmeetewinkel commented Feb 22, 2024 •

edited

Loading